Try: Set `show_in_rest` to `true` by default when `label` argument is defined #7302

SantosGuillamot · 2024-09-06T12:01:27Z

This pull request is built on top of #7298

As suggested by @gziolo here, in this pull request I'm exploring the possibility of setting the show_in_rest argument to true by default when the new potential label argument is defined. With the introduction of a new human-readable argument, which hasn't been used before, it could make sense to expose it automatically in the REST API.

Basically, these are the use cases:

If show_in_rest is defined, that value is always respected.
No label and no show_in_rest: It keeps working as before and uses show_in_rest false, which is the default. This is a pretty common use case right now.
Defined label and no show_in_rest: It sets show_in_rest to true. This would be the change introduced.

It seems I can't compare against my other branch. The relevant code changes in this PR are:

New conditional to change show_in_rest: link.
Add a new unit test to cover this use case: link.

Trac ticket: https://core.trac.wordpress.org/ticket/62000

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

gziolo · 2024-09-06T12:07:56Z

src/wp-includes/meta.php

@@ -1451,6 +1454,9 @@ function register_meta( $object_type, $meta_key, $args, $deprecated = null ) {
 	 * @param string $meta_key    Meta key.
 	 */
 	$args = apply_filters( 'register_meta_args', $args, $defaults, $object_type, $meta_key );
+	if ( isset( $args['label'] ) && ! isset( $args['show_in_rest'] ) ) {


This should change before the filter so 3rd party code can still change it.

What happens when the filter adds a label without setting show_in_rest? Shouldn't that follow the same behavior?

Maybe it is safer to move it before the filter as you say to ensure people can change this behavior.

Mamaduka · 2024-09-06T12:08:08Z

This probably needs a separate track ticket. Similar discussions usually happen there.

gziolo · 2024-09-06T12:10:22Z

I left one minor note. I'm in favor of that enhancement as it is the feedback we get several times from folks using Block Bindings that they need to remember to enable show_in_rest, but I'm happy to hear about potential consequences that make it a bad idea. I can't think of any as of today.

github-actions · 2024-09-06T12:14:18Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

SantosGuillamot · 2024-09-06T12:49:27Z

This probably needs a separate track ticket. Similar discussions usually happen there.

I've opened a separate ticket for this: https://core.trac.wordpress.org/ticket/62000#ticket.

TimothyBJacobs · 2024-09-06T17:51:36Z

I'm pretty strongly against this. Exposing a meta value can be a pretty big footgun. I don't see a compelling reason why we should make that footgun less obvious by conflating the "label" field with whether the meta value is exposed.

SergeyBiryukov · 2024-09-07T22:29:42Z

src/wp-includes/meta.php

+	if ( isset( $args['label'] ) && ! isset( $args['show_in_rest'] ) ) {
+		$args['show_in_rest'] = true;
+	}


Suggested change

if ( isset( $args['label'] ) && ! isset( $args['show_in_rest'] ) ) {

$args['show_in_rest'] = true;

}

if ( isset( $args['label'] ) && ! isset( $args['show_in_rest'] ) ) {

$args['show_in_rest'] = true;

}

Just adding some spacing for clarity.

dlh01 · 2024-09-09T05:58:56Z

I'm sympathetic to the fact that it's easy to forget to enable show_in_rest in lots of situations, but I agree with @TimothyBJacobs. As described in #7298, the purpose of the label is to have "a human-readable name that can be used across the UI" (which is great!). As a developer, I would have no expectation that taking advantage of that enhancement would potentially affect whether data is exposed to the outside world. Even setting a post type to public doesn't automatically enable show_in_rest, and that seems like as strong an indicator as could have been. So, for me, this would be an unwelcome surprise.

gziolo · 2024-09-09T07:44:55Z

Thank you for the feedback shared so far. In retrospect, I'm not surprised as it indeed could be confusing to learn that label has some special behavior when registering a new meta. It looks like what could help is having something at the level of abstraction that WP_REST_Meta_Fields is. In essence, is there a way we could allow registering a meta field, that with some new set of properties could get easily integrated into the editor and new concepts like Block Bindings, Data Views, exposed in the sidebar of the editor when editing the post type, etc. I guess the Fields API project is partially related to that but in other areas of WP Admin.

TimothyBJacobs · 2024-09-09T13:54:53Z

is there a way we could allow registering a meta field, that with some new set of properties could get easily integrated into the editor and new concepts like Block Bindings, Data Views, exposed in the sidebar of the editor when editing the post type, etc

Isn't that what register_meta_field and the REST API does? What am I missing?

gziolo · 2024-09-11T09:42:37Z

Isn't that what register_meta_field and the REST API does? What am I missing?

There is no function register_meta_field, so I assume you are referring to how the REST API works for Post Meta. At some point, we will have to revisit it and introduce some more high level utility that while registering post meta field is able to autoamtically expose it through REST API, create a block variations with ease for existing blocks with bindings applied, etc.

Anyway, I'm closing this PR for now. Thank you all for valuable feedback.

Mamaduka · 2024-09-11T10:13:44Z

I think @TimothyBJacobs was referring to the register_rest_field method.

SantosGuillamot added 8 commits September 5, 2024 16:29

Add label arg to register_meta

696b3f1

Add label to REST API schema

7593c51

Adapt unit tests

edc444c

Use 'title' instead of 'label' in schema

c307b4f

Add unit test to check title in schema

74e3465

Add since tag

0abab55

Inherit show_in_rest from label

7211ec9

Add unit test

41ae82b

SantosGuillamot mentioned this pull request Sep 6, 2024

Proposal: Add label argument to register_meta function #7298

Closed

gziolo reviewed Sep 6, 2024

View reviewed changes

SergeyBiryukov reviewed Sep 7, 2024

View reviewed changes

gziolo closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try: Set `show_in_rest` to `true` by default when `label` argument is defined #7302

Try: Set `show_in_rest` to `true` by default when `label` argument is defined #7302

SantosGuillamot commented Sep 6, 2024 •

edited

Loading

gziolo Sep 6, 2024

SantosGuillamot Sep 6, 2024

SantosGuillamot Sep 6, 2024

Mamaduka commented Sep 6, 2024

gziolo commented Sep 6, 2024

github-actions bot commented Sep 6, 2024

SantosGuillamot commented Sep 6, 2024

TimothyBJacobs commented Sep 6, 2024

SergeyBiryukov Sep 7, 2024

dlh01 commented Sep 9, 2024

gziolo commented Sep 9, 2024 •

edited

Loading

TimothyBJacobs commented Sep 9, 2024

gziolo commented Sep 11, 2024

Mamaduka commented Sep 11, 2024

Try: Set show_in_rest to true by default when label argument is defined #7302

Try: Set show_in_rest to true by default when label argument is defined #7302

Conversation

SantosGuillamot commented Sep 6, 2024 • edited Loading

gziolo Sep 6, 2024

Choose a reason for hiding this comment

SantosGuillamot Sep 6, 2024

Choose a reason for hiding this comment

SantosGuillamot Sep 6, 2024

Choose a reason for hiding this comment

Mamaduka commented Sep 6, 2024

gziolo commented Sep 6, 2024

github-actions bot commented Sep 6, 2024

Test using WordPress Playground

Some things to be aware of

SantosGuillamot commented Sep 6, 2024

TimothyBJacobs commented Sep 6, 2024

SergeyBiryukov Sep 7, 2024

Choose a reason for hiding this comment

dlh01 commented Sep 9, 2024

gziolo commented Sep 9, 2024 • edited Loading

TimothyBJacobs commented Sep 9, 2024

gziolo commented Sep 11, 2024

Mamaduka commented Sep 11, 2024

Try: Set `show_in_rest` to `true` by default when `label` argument is defined #7302

Try: Set `show_in_rest` to `true` by default when `label` argument is defined #7302

SantosGuillamot commented Sep 6, 2024 •

edited

Loading

gziolo commented Sep 9, 2024 •

edited

Loading